-
Notifications
You must be signed in to change notification settings - Fork 213
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[ADP-3226] Use more IsRecentEra
#4422
[ADP-3226] Use more IsRecentEra
#4422
Conversation
91851b2
to
24d10e0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me!
(Modulo one small comment about a $
that can be removed.)
lib/balance-tx/test/spec/Internal/Cardano/Write/Tx/BalanceSpec.hs
Outdated
Show resolved
Hide resolved
scriptData | ||
aux | ||
val | ||
removeDummyInput RecentEraConway = \case |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice 👍🏻
@@ -1198,19 +1199,21 @@ prop_sealedTxRecentEraRoundtrip | |||
sealedTxB = sealedTxFromBytes' currentEra txBytes | |||
|
|||
makeShelleyTx | |||
:: IsRecentEra era | |||
:: Write.IsRecentEra era |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why use IsRecentEra
qualified but not RecentEra
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Historical reasons only. RecentEra
was unqualified before I touched the file; I at least wanted to qualify all IsRecentEra
that I touched.
@@ -1146,13 +1146,14 @@ binaryCalculationsSpec' era = describe ("calculateBinary - "+||era||+"") $ do | |||
calculateBinary net utxo outs chgs pairs = | |||
hex (Cardano.serialiseToCBOR ledgerTx) | |||
where | |||
ledgerTx :: Cardano.Tx (CardanoApiEra era) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cannot satisfy: cardano-ledger-binary-1.2.1.0:Cardano.Ledger.Binary.Version.MinVersion <= Ledger.ProtVerHigh era0
without type annotation 🤔 maybe we should have kept the era
arg here then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was the error I saw when testing dropping era
from constructTransaction
also 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the presence of GADTs, it is good practice — and sometimes necessary — to annotate polymorphic types in where
or let
bindings.
More information can be found in the documentation for the MonoLocalBinds
extensions, which is implied by the GADTs
extension.
In other words: Using GADTs
has a cost. MonoLocalBinds
is that cost. We pay it by sometimes adding type signatures.
c17a528
to
8801dd0
Compare
Co-authored-by: Johannes Lund <anviking@me.com> Co-authored-by: Jonathan Knowles <mail@jonathanknowles.net>
8801dd0
to
4f2c631
Compare
This pull request changes some code to align with the decision "Singleton type RecentEra :: AP".
In some places, I have
RecentEra era
argument in favor of the type class / implicit argumentWrite.IsRecentEra era
— unless this would have resulted in an ambiguous type, such as formkTransaction
.@era
applications that are not of the formrecentEra @era
.(I have left applications of the form
shelleyBasedEra @era
andcardanoEra @era
for convenience, as they are similar in spirit.)Comments
This pull request is an alternative to [Experimental] Simplify the use of recent eras #4409 .
I have kept or even added an explicit
RecentEra era
argument to the functionsbuildTransaction
,constructTransaction
, andconstructUnbalancedSharedTransaction
, because their usage site (Server
module) is prone to ambiguity. Theera
type is introduced through an existential like thisIn order to get the
era
type into scope, we would need to writeIn both cases, the value or type are needed to specify in which era the
SealedTx
should be created.Issue Number
ADP-3226